Skip to content

Feature/#198 password encoding#199

Open
mplieske wants to merge 1 commit into
developfrom
feature/#198_password_encoding
Open

Feature/#198 password encoding#199
mplieske wants to merge 1 commit into
developfrom
feature/#198_password_encoding

Conversation

@mplieske

Copy link
Copy Markdown
Contributor

I added Argon2 password encoding.

Take it or leave it 😆

Any suggestions are welcome :)

Comment thread pom.xml
<!-- for Argon2PasswordEncoder -->
<dependency>
<groupId>org.bouncycastle</groupId>
<artifactId>bcprov-jdk18on</artifactId>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I would rather use a spring buildin passwordencoder like https://docs.spring.io/spring-security/reference/features/authentication/password-storage.html (I did not validate this yet.) But I like less dependencies :P

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Argon2PasswordEncoder is mentioned on the page you linked :)
https://docs.spring.io/spring-security/reference/features/authentication/password-storage.html#authentication-password-storage-argon2

To be fair, we could use the default password encoder. Probably it will be sufficient for KeepTime 😄

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right. argon is mentioned

the default one sound also sufficient - saves 8MB of disk space :D

@@ -758,12 +759,15 @@ private void handleApiOn() {
String username = authName.getText();
String password = authPassword.getText();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like you commented in the issue
"The configuration screen reads the password from the application.properties and loads it into the view. If you already have a configuration for the REST API feature and you want to change something, eg the port, the already hashed password will be hashed and saved."

Your idea -> "Probably we should only save a password if provided a new one and we should not load the password from the application.properties into the view"

sounds reasonable, yes 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants